plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java (2)

72-86: Fixture readiness check is solid; consider aligning with credentials() or documenting behavior

The @Before hook nicely encapsulates the flakiness fix by:

• Ensuring the node is green before tests run.
• Using assertBusy with a bounded timeout so test.azure.container / test.azure.base must become non‑blank within 60s or fail fast.

Two optional refinements to consider:

• To keep the readiness condition exactly in sync with what the test later requires, you could either:
  • Also include test.azure.account and the key/SAS token checks here, or
  • Call credentials() inside the assertBusy block (if it has no side effects beyond validations), so the same invariants gate fixture readiness and repository creation.
• A brief comment explaining that these system properties can transition from blank to non‑blank after JVM startup (due to the Gradle Azure fixture) would make this less “mysterious” to future readers.

These are nice‑to‑have; the current implementation already addresses the reported flakiness.

---

90-98: Double‑check interaction with superclass tearDown and robustness of wildcard deletes

The additional cleanup is directionally right (preventing state leakage across runs), but there are a couple of subtle points worth verifying:

1. Deleting all repositories before super.tearDown()
AbstractThirdPartyRepositoryTestCase defines its own tearDown() that historically:

   • Cleans up the blob store for the default "test-repo".
   • Deletes "test-repo" via the cluster admin client. (jar-download.com)

   By calling client().admin().cluster().prepareDeleteRepository("_all").get(); first, you may be removing "test-repo" before super.tearDown() runs, depending on the current framework implementation. If the superclass still assumes that repository exists (e.g., via getRepository()), super.tearDown() could start failing with RepositoryMissingException.

   To avoid that risk, consider one of:

   • Limiting deletion here to indices only and letting AbstractThirdPartyRepositoryTestCase.tearDown() remain the sole authority for repo cleanup.
   • Or, if you really need to clear all repositories, ensuring that the superclass no longer depends on "test-repo" being present (and documenting that assumption in a comment).

2. Wildcard index delete edge cases
   prepareDelete("*") can behave differently depending on index options and whether only system/hidden indices are present. In some configurations this may throw if there are no matching indices or if deletes of system indices are forbidden, which would turn into teardown failures rather than just leftover state.

   If you want teardown to be “best‑effort” rather than fail the test suite when nothing is left to delete, you could catch and ignore the specific exceptions that represent “nothing to delete / forbidden system index” while still surfacing unexpected errors.

Given this is test‑only, these adjustments are about making the cleanup more future‑proof and less coupled to internal assumptions of the base class.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: prudhvigodithi
Repo: opensearch-project/OpenSearch PR: 20112
File: server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java:73-81
Timestamp: 2025-12-02T22:44:14.799Z
Learning: In OpenSearch integration tests extending OpenSearchIntegTestCase, using `LuceneTestCase.SuppressCodecs("*")` triggers special handling that selects a random production codec from the CODECS array, while `SuppressCodecs("Asserting")` or other specific codec suppressions still allow Lucene's default codec randomization which may include the asserting codec. Use `SuppressCodecs("*")` when you need to completely avoid asserting codecs (e.g., for cross-thread StoredFieldsReader usage) while maintaining production codec test coverage.

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java (1)

70-76: Consider checking all required Azure properties in the fixture readiness check.

The waitForAzureFixtureReady() method currently verifies only test.azure.container and test.azure.base, but the credentials() method (lines 111-119) also requires test.azure.account and either test.azure.key or test.azure.sas_token. While the fixture likely sets all properties together, explicitly checking for test.azure.account would catch incomplete fixture initialization earlier and provide clearer failure diagnostics.

🔎 Optional enhancement to check all required properties

 @BeforeClass
 public static void waitForAzureFixtureReady() throws Exception {
     assertBusy(() -> {
+        assertThat(System.getProperty("test.azure.account"), not(blankOrNullString()));
         assertThat(System.getProperty("test.azure.container"), not(blankOrNullString()));
         assertThat(System.getProperty("test.azure.base"), not(blankOrNullString()));
     }, 60, TimeUnit.SECONDS);
 }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Learnt from: prudhvigodithi
Repo: opensearch-project/OpenSearch PR: 20112
File: server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java:73-81
Timestamp: 2025-12-02T22:44:14.799Z
Learning: In OpenSearch integration tests extending OpenSearchIntegTestCase, using `LuceneTestCase.SuppressCodecs("*")` triggers special handling that selects a random production codec from the CODECS array, while `SuppressCodecs("Asserting")` or other specific codec suppressions still allow Lucene's default codec randomization which may include the asserting codec. Use `SuppressCodecs("*")` when you need to completely avoid asserting codecs (e.g., for cross-thread StoredFieldsReader usage) while maintaining production codec test coverage.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Learnt from: prudhvigodithi
Repo: opensearch-project/OpenSearch PR: 20112
File: server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java:73-81
Timestamp: 2025-12-02T22:44:14.799Z
Learning: In OpenSearch integration tests extending OpenSearchIntegTestCase, using `LuceneTestCase.SuppressCodecs("*")` triggers special handling that selects a random production codec from the CODECS array, while `SuppressCodecs("Asserting")` or other specific codec suppressions still allow Lucene's default codec randomization which may include the asserting codec. Use `SuppressCodecs("*")` when you need to completely avoid asserting codecs (e.g., for cross-thread StoredFieldsReader usage) while maintaining production codec test coverage.

plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java (1)

63-63: Unused import: is matcher is not used in this file.

The static import of is from Hamcrest matchers is added but never used anywhere in the file. Consider removing it to keep imports clean.

-import static org.hamcrest.Matchers.is;

Configuration used: defaults

Review profile: CHILL

Plan: Pro

plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java (2)

67-109: Consider logging exceptions in cleanup to aid CI debugging.

The catch block at lines 106-108 silently swallows all exceptions during Azure cleanup. While the comment indicates this is intentional for CI teardown, completely silent failures make debugging difficult when cleanup issues occur.

Additionally, line 83 uses an empty string default "" for test.azure.key when the property is missing. Although the fixture should ensure properties are set, being more defensive (e.g., checking the property exists or at least logging when using defaults) would make the code more maintainable.

🔎 Optional improvements for cleanup resilience

         } catch (Exception ignored) {
-            // CI teardown
+            // CI teardown - log but don't fail the test run
+            System.err.println("Warning: Azure blob cleanup failed: " + ignored.getMessage());
         }

For line 83, consider validating the property exists or using a more explicit pattern:

-                secureSettings.setString("azure.client.default.key", System.getProperty("test.azure.key", ""));
+                String key = System.getProperty("test.azure.key");
+                if (Strings.hasText(key)) {
+                    secureSettings.setString("azure.client.default.key", key);
+                }

---

133-156: Redundant empty string defaults after property validation.

Lines 141 and 143 use empty string defaults "" when retrieving test.azure.sas_token and test.azure.key. However, the first assertBusy block (lines 119-131) has already validated these properties are properly set. Using empty string defaults here is redundant and could mask issues if the validation logic changes.

🔎 Optional: Remove redundant defaults for clarity

             if (hasSasToken) {
-                secureSettings.setString("azure.client.default.sas_token", System.getProperty("test.azure.sas_token", ""));
+                secureSettings.setString("azure.client.default.sas_token", System.getProperty("test.azure.sas_token"));
             } else {
-                secureSettings.setString("azure.client.default.key", System.getProperty("test.azure.key", ""));
+                secureSettings.setString("azure.client.default.key", System.getProperty("test.azure.key"));
             }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Learnt from: prudhvigodithi
Repo: opensearch-project/OpenSearch PR: 20112
File: server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java:73-81
Timestamp: 2025-12-02T22:44:14.799Z
Learning: In OpenSearch integration tests extending OpenSearchIntegTestCase, using `LuceneTestCase.SuppressCodecs("*")` triggers special handling that selects a random production codec from the CODECS array, while `SuppressCodecs("Asserting")` or other specific codec suppressions still allow Lucene's default codec randomization which may include the asserting codec. Use `SuppressCodecs("*")` when you need to completely avoid asserting codecs (e.g., for cross-thread StoredFieldsReader usage) while maintaining production codec test coverage.

plugins/repository-azure/src/internalClusterTest/java/org/opensearch/repositories/azure/AzureStorageCleanupThirdPartyTests.java (3)

73-84: Validate authentication property before cleanup to avoid confusing errors.

The early return on lines 73-75 checks only container and account, but when test.azure.sas_token is not set, line 83 will use an empty string default for test.azure.key. If the key property is genuinely missing, passing an empty string to secure settings may cause authentication failures that are harder to diagnose than an early null check.

🔎 Suggested improvement

-        if (Strings.isNullOrEmpty(container) || Strings.isNullOrEmpty(account)) {
+        boolean hasSasToken = Strings.hasText(System.getProperty("test.azure.sas_token"));
+        String key = System.getProperty("test.azure.key");
+        
+        if (Strings.isNullOrEmpty(container) || Strings.isNullOrEmpty(account)) {
             return;
         }
+        
+        if (!hasSasToken && Strings.isNullOrEmpty(key)) {
+            return;
+        }

         MockSecureSettings secureSettings = new MockSecureSettings();
         secureSettings.setString("azure.client.default.account", account);

-        if (Strings.hasText(System.getProperty("test.azure.sas_token"))) {
+        if (hasSasToken) {
             secureSettings.setString("azure.client.default.sas_token", System.getProperty("test.azure.sas_token"));
         } else {
-            secureSettings.setString("azure.client.default.key", System.getProperty("test.azure.key", ""));
+            secureSettings.setString("azure.client.default.key", key);
         }

---

100-102: Consider logging cleanup failures instead of silently swallowing all exceptions.

The catch-all exception handler on line 100 silently ignores all failures during Azure blob cleanup. While robust cleanup is important for CI, completely silent failures can hide configuration issues, authentication problems, or actual bugs in the cleanup logic itself.

🔎 Suggested improvement

Consider logging at minimum:

-        } catch (Exception ignored) {
-            // CI teardown
+        } catch (Exception e) {
+            // Log but don't fail the build during CI teardown
+            logger.warn("Failed to cleanup Azure blobs during test teardown", e);
         }

Or if truly all failures should be ignored, document why:

         } catch (Exception ignored) {
-            // CI teardown
+            // Ignore all cleanup failures to prevent test failures in CI when Azure fixture
+            // is unavailable or credentials expired after test completion
         }

---

126-149: Remove unnecessary empty string defaults after property validation.

Lines 134 and 136 provide empty string defaults to System.getProperty(), but the first assertBusy block (lines 112-124) already validated that these properties are present and non-empty. The empty string defaults suggest the code doesn't trust its own validation and could mask bugs if properties somehow disappear between checks.

🔎 Suggested improvement

             final boolean hasSasToken = Strings.hasText(System.getProperty("test.azure.sas_token"));
             if (hasSasToken) {
-                secureSettings.setString("azure.client.default.sas_token", System.getProperty("test.azure.sas_token", ""));
+                secureSettings.setString("azure.client.default.sas_token", System.getProperty("test.azure.sas_token"));
             } else {
-                secureSettings.setString("azure.client.default.key", System.getProperty("test.azure.key", ""));
+                secureSettings.setString("azure.client.default.key", System.getProperty("test.azure.key"));
             }

This way, if the properties are unexpectedly null, you'll get a clear NPE rather than a confusing authentication error from an empty credential.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Learnt from: prudhvigodithi
Repo: opensearch-project/OpenSearch PR: 20112
File: server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java:73-81
Timestamp: 2025-12-02T22:44:14.799Z
Learning: In OpenSearch integration tests extending OpenSearchIntegTestCase, using `LuceneTestCase.SuppressCodecs("*")` triggers special handling that selects a random production codec from the CODECS array, while `SuppressCodecs("Asserting")` or other specific codec suppressions still allow Lucene's default codec randomization which may include the asserting codec. Use `SuppressCodecs("*")` when you need to completely avoid asserting codecs (e.g., for cross-thread StoredFieldsReader usage) while maintaining production codec test coverage.